feat(crowdin): allow source string edits and deletes - #21
Merged
Conversation
The validator was an additions-only gate: any PR that edited or deleted an
existing string failed, on the policy that Crowdin was the sole origin for
those changes. Make the repo a legitimate origin for all three.
crowdin_validator.py now splits its findings in two. Edits, deletes and
renames become NOTEs -- reported with old -> new text so a reviewer can see
what a PR touched, but no longer failing the build. What still fails is a
mistake: a key defined twice in one file, an entry the parser cannot
round-trip, or an entry with an empty value. Cross-file duplicate keys keep
failing too, though the message no longer claims Crowdin rejects them (53
such pairs already exist on main). Structural checks are scoped to the files
a PR changed, so a pre-existing quirk elsewhere cannot fail an unrelated PR.
Deleting strings is allowed; deleting the file that holds them is not, and
that stays an error. `crowdin upload sources` can only ever upload a file, so
a removed .po survives in Crowdin and the next pull restores it along with
every string in it -- retiring a whole file has to start in the Crowdin UI.
Emptying a file in place is the same outcome by another route and is caught
the same way, though only for a file that had entries to begin with, since
licenses.po ships empty. Such a file is reported once rather than as a flood
of individual delete notices.
Renames are detected by pairing a deleted key with an added key carrying
identical text, and called out separately: that is the one delta where
Crowdin's key matching means the translations do not survive.
Relaxing the validator alone would have been cosmetic, so two coupled fixes
come with it. crowdin.yml gains update_option: update_as_unapproved -- these
files carry X-Crowdin-SourceKey: msgstr, so editing a msgstr is a source-text
change, and Crowdin's default would have discarded that string's translations
in every language. crowdin_sync.py warns when a whole .po is deleted: the
validator rejects that on the PR, but it skips the sync bot's own branch and
only runs on PRs, so the push keeps a backstop for anything that got in
another way.
The "never pass --delete-obsolete" comment was right but for the wrong
reason, and is corrected: per crowdin upload sources --help (CLI 4.12.0) the
flag deletes obsolete *files and folders*, not strings, so against our
per-file `-s` invocation it would have deleted the other 29 .po files.
Per-string deletion already propagates via the default --auto-update.
Adds scripts/test_crowdin_validator.py -- the repo's first tests. Stdlib
unittest, no new dependencies: parsing/detection unit tests plus end-to-end
cases that build a throwaway git repo and assert on exit code and stdout,
which is the whole contract Bitrise consumes.
python3 -m unittest discover -s scripts -p 'test_*.py'
added 3 commits
August 25, 2026 11:31
changed_po_files() diffed with git's default rename detection, which reports only the destination path for a `git mv`. Renaming a whole .po therefore hid the vanished source from removed_file(): the rename passed as a pile of "added" strings, exactly the whole-file removal the check exists to reject. The push made it worse. It uploaded the destination and left the original in Crowdin -- warn_deleted_files() filtered on D, which a rename never produces -- so the next pull restored both catalogs and every string existed twice, in two files, colliding. Pass --no-renames in all three diffs. A rename is then a delete plus an add, which is what it is as far as Crowdin is concerned: there is no rename operation on the push side, only an upload of the new path. The removal surfaces normally, and so does a move *out* of strings/en/, which rename detection otherwise hid from the path-scoped diff. crowdin_sync.py's upload filter drops R along with it, since --no-renames can no longer produce one. Caught in review of #21.
--no-renames made a renamed catalog visible, but flattened it into the same
"deleted" message as a genuine removal. The two need different fixes -- a
rename strands the old file in Crowdin *and* uploads the new one, so the pull
brings back both and duplicates every string, while a delete merely comes
back -- so the report now says which happened, and where the file went.
renamed_po_files() is a second, reporting-only diff with rename detection
back on. It is deliberately unscoped: a move out of strings/en/ shows up as
a plain delete under the path-scoped diff, because the destination no longer
matches the pathspec, and naming the destination is the whole point.
changed_po_files() keeps --no-renames, so correctness still rests on the
decomposed delete-plus-add.
strings/en/movies.po -> strings/en/films.po (12 string(s))
strings/en/fonts.po -> strings/archive/fonts.po (4 string(s))
strings/en/discover.po (deleted, 2 string(s))
strings/en/plans.po (emptied, 1 string(s))
removed_file() becomes missing_file() and returns the kind alongside the
detail. Both kinds still fail the build.
Also fixes shadowing this surfaced: the per-file string-rename list reused
the name `renames`, clobbering the file-level rename map from the previous
iteration and crashing on the second changed file. Caught by the new
mixed-case test; the file-level map is now `file_renames`.
missing_file() keyed off entries lost, so a .po with none looked like nothing to lose: `git rm strings/en/licenses.po` passed with exit 0, and renaming it passed too. licenses.po is empty but is still a tracked file, and the rule is about the file, not its contents -- a catalog on the base branch has to still be there whether it held 600 strings or none. Test with `git cat-file -e` against the base ref instead. base_content() returning "" cannot distinguish "not tracked" from "tracked but empty", which is the ambiguity that opened the hole. A path absent from the base branch is still exempt, so adding a new catalog stays fine. Emptying a file in place is the one variant that still turns on contents: it only counts as a loss if there were entries to lose. README states the rule outright -- never delete, rename or move a .po.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
scripts/crowdin_validator.pywas an additions-only gate: any PR that edited or deleted an existing string failed, on the policy that Crowdin was the sole origin for those changes. This makes the repo a legitimate origin for all three.Findings now split in two:
Structural checks are scoped to the files a PR changed, so a pre-existing quirk elsewhere can't fail an unrelated PR.
Deleting strings is allowed; deleting the file that holds them is not.
crowdin upload sourcescan only ever upload a file, so a removed.posurvives in Crowdin and the next pull restores it along with every string in it. Emptying a file in place is the same outcome by another route and is caught the same way — but only for a file that had entries to begin with, sincelicenses.poships empty. Such a file is reported once rather than as a flood of per-string delete notices.Renames are detected by pairing a deleted key with an added key carrying identical text, and called out separately: that's the one delta where Crowdin's key matching means translations don't survive.
Two coupled fixes, without which the above would be cosmetic
crowdin/crowdin.ymlgainsupdate_option: update_as_unapproved. These files carryX-Crowdin-SourceKey: msgstr, so editing amsgstris a source-text change. Crowdin's default would have discarded that string's translations in every language. Validated against the CLI's config parser, with a deliberately-invalid control to confirm the check actually runs.crowdin_sync.pywarns when a whole.pois deleted. The validator rejects that on the PR, but it skips the sync bot's own branch and only runs on PRs, so the push keeps a backstop.The
# Never pass --delete-obsoletecomment was right but for the wrong reason, and is corrected. Percrowdin upload sources --help(CLI 4.12.0) the flag deletes obsolete files and folders, not strings — against our per-file-sinvocation it would have deleted the other 29.pofiles. Per-string deletion already propagates via the default--auto-update.Also softened: the cross-file duplicate-key message no longer claims Crowdin rejects them. Crowdin scopes keys per file and tolerates it — 53 such pairs already exist on
main(about,back,done, …). It still fails the build because it's almost always a copy-paste slip, but it only ever fires on newly-added keys, so no existing content is affected.Tests
Adds
scripts/test_crowdin_validator.py— the repo's first tests. Stdlibunittest, no new dependencies.python3 -m unittest discover -s scripts -p 'test_*.py'36 cases: parsing/detection unit tests, plus end-to-end cases that build a throwaway git repo and assert on exit code and stdout, which is the whole contract Bitrise consumes. Includes merge shapes (delete + edit, two-into-one, cross-file) to pin that a merge is never misread as a rename, and boundary cases for the whole-file rule.
Verification
strings/en/.movies.po(12 strings) + emptyingfonts.po(4 strings) → exit 1, both named with counts. Deleting two strings fromapp.powhile keeping the file → exit 0.--push --dry-run: correct per-file scoping, no--delete-obsolete, whole-file-delete warning fires.Reviewer notes
AGENTS.md. The convention isinitials/description-TICKET-IDwith the ticket last; this branch has no ticket ID, anddeltesis a typo fordeletes. Flagged here per.claude/rules/branch-naming.mdrather than silently renaming, since the name was chosen deliberately. Happy to rename if you'd prefer.CROWDIN_API_TOKENagainst shared project 257. Everything else was verified offline, but the reconcile-on-upload behaviour relied on for per-string deletes is inferred from the CLI's documented--auto-update, not observed live.youversion-flutter-loop/scripts/crowdin_validator.pyis a near-identical copy whosevalidate()was byte-for-byte the same. It still blocks edits and deletes for that repo — separate PR.Greptile Summary
The PR allows repository-originated source-string additions, edits, and deletions while retaining structural validation and preventing whole-catalog loss.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Repository source-string change] --> B[Catalog validator] B -->|Valid string-level change| C[Merge] B -->|Structural error or catalog loss| D[Reject PR] C --> E[Crowdin source upload] E --> F[Updated Crowdin source state] F --> G[Future source pull] G --> AReviews (4): Last reviewed commit: "fix(crowdin_validator): reject losing a ..." | Re-trigger Greptile